Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I have added new BMP280 barometer support in cleanflight #975

Closed
wants to merge 9 commits into from

Conversation

rlehey
Copy link

@rlehey rlehey commented Jun 1, 2015

It builds

@JohnieBraaf
Copy link
Contributor

Why did you add it to makefile and target.h for all the boards? Should you not just add it only to the boards that actually have this sensor?

@rlehey
Copy link
Author

rlehey commented Jun 1, 2015

I follow the bmp085 which is a similar sensor.
It could be a replacement.
There is no target specific code, so it doesn't hurt to build it.

@rlehey
Copy link
Author

rlehey commented Jun 1, 2015

Also it can connect by breakout board to any target with i2c. This is how I test it.

@JohnieBraaf
Copy link
Contributor

I would say you only define the sensors actually present on the target.
Otherwise you should also add all the other ones no? Although CC3D has multiple indeed.

@rlehey
Copy link
Author

rlehey commented Jun 1, 2015

See my comment just before your. It can connect to any target using I2C breakout.

@JohnieBraaf
Copy link
Contributor

Yeah, that makes sense

@JohnieBraaf
Copy link
Contributor

How do you make cleanflight pick the right sensor?

@rlehey
Copy link
Author

rlehey commented Jun 1, 2015

It's automatically detected, the case-switch in sensors/initialization.c, if not detected then it will try another or else disable that type of sensor. So without it connected also still works.

@rlehey
Copy link
Author

rlehey commented Jun 13, 2015

What's the status on this?

@developingchris
Copy link

I have a board with a bmp280, that I'd love to get support for. What needs to happen to this, for it to be in the next release?
@JohnieBraaf ?

@hydra
Copy link
Contributor

hydra commented Jun 18, 2015

There are older existing pull requests that need to be addressed first, there's quite a backlog.

Bugs are prioritized over new features too.

@hydra
Copy link
Contributor

hydra commented Jun 18, 2015

Oh, and i need to be able to test the code before its merged but i dont have a board with a BMP280 here. Unit tests would be good too.

@rlehey
Copy link
Author

rlehey commented Jun 18, 2015

http://www.watterott.com/en/BMP280-Breakout is what I used

@developingchris
Copy link

@hydra being rather new to this space, is there anything I can do to help with those? Can I tag the ones with bad travis builds or something, in a way to triage bugs that can be paid attention to?

@hydra
Copy link
Contributor

hydra commented Jun 18, 2015

Yes, submittig PRs that fix the bugs would help immensely.

@stronnag
Copy link
Contributor

@rlehey , you may also consider that somewhat less disingenuously describing the target of this patch may help merging into main line. Specifically, rather than spamming all targets, creating a dedicated target (such as DODOF3) may more accurately identify the real purpose of the PR and thus hasten its acceptance. I would council against the terms such as SERIOUSLY or WOW in creating such a target if you intend the PR to be taken, dare I say it, seriously.

@rlehey
Copy link
Author

rlehey commented Jun 20, 2015

the sensor can be used by other targets with breakout boards.

the same way there is BMP 085/180 support now. anyone making a commercial
for profit board with that?
On Jun 21, 2015 2:41 AM, "stronnag" notifications@github.com wrote:

@rlehey https://github.com/rlehey , you may also consider that somewhat
less disingenuously describing the target of this patch may help merging
into main line. Specifically, rather than spamming all targets, creating a
dedicated target (such as DODOF3) may more accurately identify the real
purpose of the PR and thus hasten its acceptance. I would council against
the terms such as SERIOUSLY or WOW in creating such a target if you intend
the PR to be taken, dare I say it, seriously.


Reply to this email directly or view it on GitHub
#975 (comment)
.

@rlehey
Copy link
Author

rlehey commented Jun 21, 2015

my PR adds support for the barometer.
I have no idea what dedicated target you are talking about. I've added it
into every target that had i2c headers where external board can be
attached. last I checked committing support for new hardware didn't require
having commercial interest in a particular target.
On Jun 21, 2015 2:41 AM, "stronnag" notifications@github.com wrote:

@rlehey https://github.com/rlehey , you may also consider that somewhat
less disingenuously describing the target of this patch may help merging
into main line. Specifically, rather than spamming all targets, creating a
dedicated target (such as DODOF3) may more accurately identify the real
purpose of the PR and thus hasten its acceptance. I would council against
the terms such as SERIOUSLY or WOW in creating such a target if you intend
the PR to be taken, dare I say it, seriously.


Reply to this email directly or view it on GitHub
#975 (comment)
.

@thenickdude
Copy link
Contributor

AfroMini revision 3 is now shipping with this BMP280 barometer.

@FenomPL
Copy link

FenomPL commented Jul 8, 2015

@hydra Any chance to merge it soon?

@theantnest
Copy link

Hey guys, would appreciate this to be finalised/ fixed/ merged. Thanks for all your hard work

@hydra
Copy link
Contributor

hydra commented Jul 12, 2015

The servo/mixer code and related configurator changes to support the tilt flight modes are currently the highest priority.

Some unit tests for this PR wouls help immensely.

@theantnest
Copy link

Please let me know if there's any way I can help out (in any way - not just related to this pull request). I know screw all about coding, but have a pretty decent lab kitted up for hardware, and am no noob about firmware and protocols.

@thenickdude
Copy link
Contributor

@theantnest unit tests that check for datasheet-specified conversions between barometer raw readings and the output pressures/temperatures would be the main testable component here. The preexisting baro_bmp085_unittest.cc can be used as a template.

@add1ct3dd
Copy link
Contributor

Any progress on this? Could do with this being in the next release (imo).

@leothehuman
Copy link

I have checked this out on afromini rev3, the code works, nothing is broken as far as I can see. The values it outputs look correct, but very imprecise.
Changes look like a port from baseflight.
I would suggest to limit the impact to naze boards, as attaching separate bmp280 seems unlikely.
I would consider baro not working on afromini rev3 as a bug and give it a priority.

@xoxota99
Copy link

What's blocking this merge? Is it just the backlog of pull requests? Something else?

@leothehuman
Copy link

@hydra please, consider accepting this. It works and helps people with rev3 afrominis.

@flacjacket
Copy link
Contributor

I've taken this code and added the requisite unit tests, see #1316.

@hydra
Copy link
Contributor

hydra commented Oct 6, 2015

merged.

@hydra hydra closed this Oct 6, 2015
martinbudden pushed a commit to martinbudden/cleanflight that referenced this pull request Aug 11, 2016
sambas pushed a commit to sambas/cleanflight that referenced this pull request Dec 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet